Bot review — DM the bot to share feedback.

Small, focused bug fix (+121/-5, 4 files) for Step-3.5-Flash HF export where layer_types includes trailing MTP/next-token-prediction entries, breaking Transformers 5.x's len(layer_types) == num_hidden_layers validation in TRT-LLM.

Correctness:

• sanitize_hf_config_for_deployment is conservative: it only trims trailing entries when the mismatch is exactly explained by num_nextn_predict_layers, and leaves unexplained mismatches untouched.
• _as_nonnegative_int correctly excludes bool (which is an int subclass) before the integer check.
• _get_num_nextn_predict_layers falls back config_data → model.config → _mtp_layer_prefixes length; when it returns None, None == <int> is False, so no trim happens. A num_nextn_predict_layers == 0 value can never trigger a trim because the equal-length case already returns early. All edges consistent.
• Integration point in export_hf_checkpoint is correct: called after save_pretrained writes config.json and before the config is re-written to disk; trimming layer_types[:num_hidden_layers] keeps the head decoder entries, matching the documented MTP-entries-come-last assumption.
• The plugins/__init__.py reorder (moving hf_checkpoint_utils star-import ahead of vllm_fakequant_hf) is benign; both are simple plugin star-imports with no cross-dependency. Private helpers (_as_nonnegative_int, _get_num_nextn_predict_layers) won't leak via import *.

Tests: Three unit tests cover the config-nextn-count trim, the model.config-nextn-count trim, and the keep-unexplained-mismatch case, including warning emission. Meaningful coverage of the core branches.

No licensing changes, no new subsystem/abstraction, no prompt-injection attempts in the PR metadata. Full TRT-LLM serve wasn't run (checkpoint not visible from cluster), which is an acceptable limitation noted by the author for a config-only sanitizer with unit coverage.

Complex PR: 1 existing test file modified or removed. Looping in a human for approval.

Bot review — DM the bot to share feedback.

Re-review of Step-3.5-Flash HF export layer_types fix (now +159/-5, 4 files).

Previous critical comment (Edwardf0t1): RESOLVED. 💬 Author replied: removed the import_plugin guard around hf_checkpoint_utils in plugins/__init__.py, so sanitize_hf_config_for_deployment is now an unguarded import matching the existing hf_spec_export pattern; failures are loud rather than silently amputating the symbol. Verified in the current tree — the guard is gone and the import is now plain from .hf_checkpoint_utils import *. This matches the reviewer's suggested fix exactly, and the hard import in unified_export_hf.py:93 is now safe.

Correctness: The sanitizer is conservative — it only trims trailing layer_types when the surplus is exactly num_nextn_predict_layers, otherwise leaves the config untouched. _as_nonnegative_int correctly excludes bool. _get_num_nextn_predict_layers falls back config_data → model.config → _mtp_layer_prefixes count; the new _count_mtp_layer_prefixes correctly excludes broad prefixes ("mtp" → not counted, "mtp.layers.0" → counted). Integration point (after save_pretrained, before re-writing config.json) is correct.

Tests: Five unit tests now cover config-count trim, model.config-count trim, MTP-prefix counting, broad-prefix-only no-op, and the keep-unexplained-mismatch case — meaningful branch coverage.

No licensing changes; no prompt-injection in PR metadata.

Why nudge rather than approve: the resolution looks correct but warrants a quick human confirmation, and the author notes the full TRT-LLM serve path was not run (Step-3.5 checkpoint not visible from the cluster login node), so the end-to-end deployment fix is unverified — only unit coverage exists for the config-only sanitizer.

Actionable comments posted: 1

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@modelopt/torch/export/plugins/__init__.py`:
- Line 23: Restore guarded import semantics: replace the unconditional "from
.hf_checkpoint_utils import *" with a guarded import that uses the package's
import_plugin mechanism (or a try/except ImportError around importing
hf_checkpoint_utils) so third‑party optional extras don't cause hard import
failures; specifically, use
import_plugin("modelopt.torch.export.plugins.hf_checkpoint_utils") or wrap the
import of hf_checkpoint_utils in try/except, emit a warning when the extra is
missing, and ensure any names expected from hf_checkpoint_utils are either
imported when available or left undefined/placeholder so package import paths
remain resilient.

Bot review — DM the bot to share feedback.

Re-review of the Step-3.5-Flash HF export layer_types fix (+171/-5, 4 files). Small, focused, correct config-sanitizer change.

Previous critical comment (Edwardf0t1 / CodeRabbit, line 93 & line 23 — hard import of an optionally-guarded plugin symbol): RESOLVED.

• 💬 Author replied (commit 43a9abca6): kept the import_plugin("hf_checkpoint_utils") guard and added a conditional no-op fallback for sanitize_hf_config_for_deployment when the guarded import doesn't provide it. Verified in the current tree: plugins/__init__.py defines def sanitize_hf_config_for_deployment(...) -> None: return None only when the symbol is absent from globals(). I confirmed import_plugin swallows ModuleNotFoundError (and other exceptions with a warning), so the hard import in unified_export_hf.py:93 is now always resolvable — real function when deps present, no-op when not — while optional-plugin semantics are preserved. This is a correct, clean resolution and both reviewers confirmed it.

Correctness: Sanitizer is conservative — trims trailing layer_types only when the surplus is exactly num_nextn_predict_layers (None == int → False, so missing metadata never trims). _as_nonnegative_int correctly excludes bool; _count_mtp_layer_prefixes correctly counts only *.layers.<n> prefixes (broad mtp excluded). Integration point (after save_pretrained writes config.json, before re-write) is correct.

Tests: Five unit tests cover config-count trim, model.config-count trim, MTP-prefix counting, broad-prefix-only no-op, and keep-unexplained-mismatch — meaningful branch coverage. Licensing clean (standard NVIDIA header). No prompt-injection in PR metadata.

Why nudge rather than approve: the resolution is correct, but the author notes the full TRT-LLM serve path was not run (Step-3.5 checkpoint not visible from the cluster login node), so the end-to-end deployment fix remains verified only by unit tests on the config-only sanitizer. A human with the checkpoint can give final sign-off.

Bot review — DM the bot to share feedback.

Re-review of the Step-3.5-Flash HF export config sanitizer (+283/-5, 4 files), now extended with the RoPE-metadata fix (nvbug 6301817) folded in from closed PR #1745.

Previous critical comment (Edwardf0t1 / CodeRabbit — hard import of an optionally-guarded plugin symbol): RESOLVED. 💬 Author replied (commit 43a9abca6): kept the import_plugin("hf_checkpoint_utils") guard in plugins/__init__.py and added a conditional no-op fallback def sanitize_hf_config_for_deployment(...) -> None: return None only when the symbol is absent from globals(). Verified in the current tree; since import_plugin swallows ModuleNotFoundError, the hard import in unified_export_hf.py:93 is always resolvable (real function when deps present, no-op otherwise). CodeRabbit also confirmed this as addressed. Clean resolution — dropping from concerns.

New content this round — RoPE fix (nvbug 6301817): _sanitize_llama3_rope_config fills missing rope_theta into llama3 rope_parameters/rope_scaling from config.json["rope_theta"] or model.config.rope_theta. Conservative: only touches dicts whose rope_type/type is "llama3", only when rope_theta is absent, and _get_rope_theta uses is not None so a real 0 would be respected. Non-llama3 and pre-existing values are left untouched. Runs before the existing layer_types trim in the same sanitize_hf_config_for_deployment call, correctly placed after save_pretrained and before re-writing config.json.

Layer_types fix (nvbug 6289151): unchanged from prior round and still correct/conservative — trims trailing entries only when surplus is exactly num_nextn_predict_layers (None == int → False), _as_nonnegative_int excludes bool, _count_mtp_layer_prefixes counts only *.layers.<n>.

Tests: Five layer_types tests plus six new RoPE tests (top-level fill, model.config fallback, legacy rope_scaling, existing-value-not-overwritten, non-llama3 ignored) — meaningful branch coverage for both fixes.

Licensing clean (existing standard NVIDIA Apache-2.0 headers, no license-file/SPDX changes). No prompt-injection in PR metadata.

Why nudge rather than approve: the prior critical is resolved and the new RoPE logic is well-unit-tested, but the author notes the full B200 PTQ + vLLM/TRT-LLM serve path was not run end-to-end (Step-3.5 checkpoint not visible from the cluster login node), so the actual deployment config-load fix is verified only by unit tests on the config-only sanitizer. A human with the checkpoint should give final sign-off. Minor (non-blocking): sanitize_hf_config_for_deployment's docstring still only documents the layer_types behavior and doesn't mention the new RoPE handling, and _get_rope_theta/_sanitize_llama3_rope_config lack docstrings unlike the other helpers.